-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
guard against past-end iterator invalidation #1247
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code patches a symptom without addressing the root cause. The program will stop crashing at this exact location, but it will still produce corrupted output because we need to assign a UV set name to each one of the inputs declared in the material:
def Material "foo1SG_map1_uvSet1" (kind = "assembly")
{
token inputs:file1:varname = "map1" // Potentially incorrect
token inputs:file2:varname = "uvSet1" // Potentially incorrect
token inputs:file3:varname = // Missing
...
}
I would be very interested in seeing a sample scene that reproduces the crash. It is possible that we want to detect layered materials and save only the active layer if exporting for UsdPreviewSurface.
Generating a simple scene that reproduces the crash will also allow adding it as a unit test. There are a few materials in test\lib\usd\translators\UsdExportUVSetMappingsTest\UsdExportUVSetMappingsTest.ma that could probably be layered on a new plane to reproduce the issue quickly.
Hi @JGamache-autodesk after a bit of investigation the following case will trigger the crash: Basically having two I haven't yet looked into what exactly all the code surrounding material mappings is doing, so I can't speak to the issue of generating corrupted output. But I still think adding this condition (or an assert) makes sense since there isn't any reasonable way to continue if the array sizes are mismatched. |
That is interesting. The export code is based on the expectations of names being unique. This expectation has been proven wrong in this scenario when using non-namespaced names. The correct fix, if you want to attempt it, is to use the full namespaced name for This should work fine downstream since you already made a fix in PR #1062 to handle the extra |
- in case of mismatch, just assign most common uv set mapping to material inputs
Hi, thanks for taking a look. I might be missing something here because even
Nevertheless your suggestion makes sense, so I went ahead and put the absolute
I updated the code in this case to always assign the most-common mapping to the |
I've uploaded a test scene that should reproduce the issue when exported with the following command:
I just double-checked against 991913f (current top of Hopefully you are able to reproduce this! If not, let me know and I can try to investigate further. |
I can reproduce the crash. Thanks for providing a file. |
Remove the |
… instead and just assert for iterator invalidation
Hi @JGamache-autodesk : thanks for looking into this! I've updated the PR accordingly by avoiding having the namespace removed and limited the earlier changes to just a |
Hi:
This PR fixes a crash we're seeing on some exports where layered shaders are involved, leading to a mistaken assumption in array sizes and resulting in past-the-end-iterator invalidation.
Please review and raise any concerns.